Skip to content

Don't throw InexactError for comparisons between FDs of different types and/or Integers#88

Merged
NHDaly merged 13 commits intomasterfrom
nhd-equality-no-inexacts
Dec 19, 2023
Merged

Don't throw InexactError for comparisons between FDs of different types and/or Integers#88
NHDaly merged 13 commits intomasterfrom
nhd-equality-no-inexacts

Conversation

@NHDaly
Copy link
Copy Markdown
Member

@NHDaly NHDaly commented Dec 19, 2023

Fixes #82.

Before:

FD{Int8, 0}(2) != FD{Int8, 2}(1)  # Throws because FD{Int8,2}(2) doesn't fit
2 != FD{Int8, 2}(1)  # Throws because FD{Int8,2}(2) doesn't fit
2 >= FD{Int8, 2}(1)  # Throws because FD{Int8,2}(2) doesn't fit

After, these all return true. :)

The idea in this PR is to check whether the values would fit in eachother's types, and if not, that's your answer, rather than throwing an InexactError if they fail to promote.

@NHDaly NHDaly added the bug label Dec 19, 2023
@NHDaly NHDaly marked this pull request as ready for review December 19, 2023 17:47
@NHDaly
Copy link
Copy Markdown
Member Author

NHDaly commented Dec 19, 2023

Note that this PR doesn't address InexactError caused by promotion during arithmetic, e.g.:

julia> FixedDecimal{Int8,2}(1) + 2  # Same here: 2 is promoted to FD{Int8,2}(2)
ERROR: InexactError: convert(FixedDecimal{Int8, 2}, 2)

I think that's okay for now. If we fixed it, this would either throw an OverflowError anyway if you used checked arithmetic, or it would wrap, using the new semantics. We should probably add support for this, but at least at RAI we're using checked arithmetic for everything so this is going to throw either way and so it seems less urgent to fix it to me.

mcmcgrath13
mcmcgrath13 reviewed Dec 19, 2023
View reviewed changes
mcmcgrath13
mcmcgrath13 reviewed Dec 19, 2023
View reviewed changes
mcmcgrath13
mcmcgrath13 reviewed Dec 19, 2023
View reviewed changes
@NHDaly NHDaly requested a review from mcmcgrath13 December 19, 2023 20:22
@NHDaly
Copy link
Copy Markdown
Member Author

NHDaly commented Dec 19, 2023

Thanks for the quick review! Ready for another round 😊

mcmcgrath13
mcmcgrath13 approved these changes Dec 19, 2023
View reviewed changes
Copy link
Copy Markdown

@mcmcgrath13 mcmcgrath13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ thank you!! Can you just make sure we have full test coverage before merging?

@NHDaly NHDaly force-pushed the nhd-equality-no-inexacts branch from 8369395 to 7621196 Compare December 19, 2023 20:51
@NHDaly
Copy link
Copy Markdown
Member Author

NHDaly commented Dec 19, 2023

Thanks Mary. 100% diff coverage now. (That's a really helpful tool.)

Here I go! :) I'm gonna also mark this as a patch version bump, since it's a strict improvement.

@NHDaly NHDaly merged commit 507185c into master Dec 19, 2023
@NHDaly NHDaly deleted the nhd-equality-no-inexacts branch December 19, 2023 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@mcmcgrath13 mcmcgrath13 mcmcgrath13 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

== should not leak promotion failure

2 participants

@NHDaly @mcmcgrath13